Add read_and_decode API based iis3dwb sensor driver #90286

avisconti · 2025-05-21T13:48:09Z

The IIS3DWB is a ST 3-axis digital vibration sensor. This PR includes support using read_and_decode APIs for both one shot and streaming capability. The current triggers supported are:

    - SENSOR_TRIG_FIFO_WATERMARK
    - SENSOR_TRIG_FIFO_FULL
    - SENSOR_TRIG_DATA_READY

This driver DOES NOT provide support for old fetch_and_get APIs, therefore following sensor_driver_api callbacks (as well as trigger's threads support) are not provided:

   sensor_trigger_set_t trigger_set;
   sensor_sample_fetch_t sample_fetch;
   sensor_channel_get_t channel_get;

The driver has been tested on a nucleo_h503rb board with a spi-configured x_nucleo_iks01a3 shield. It should be in line with what discussed in #70651

teburd · 2025-05-22T16:18:29Z

drivers/sensor/st/iis3dwb/Kconfig

+	select SPI
+	select HAS_STMEMSC
+	select USE_STDC_IIS3DWB
+	select RTIO_WORKQ if SENSOR_ASYNC_API


RTIO_WORKQ should be avoided if possible being selected by every driver, unless the driver is actually using it

teburd · 2025-05-22T16:19:47Z

drivers/sensor/st/iis3dwb/iis3dwb.c

+
+void iis3dwb_submit(const struct device *dev, struct rtio_iodev_sqe *iodev_sqe)
+{
+	struct rtio_work_req *req = rtio_work_req_alloc();


Work queue seems unneeded here as both one shot and stream paths are fully asynchronous already, unless I've missed something

The IIS3DWB is a system-in-package featuring a 3-axis digital vibration sensor with low noise over an ultrawide and flat frequency range. The wide bandwidth, low noise, very stable and repeatable sensitivity, together with the capability of operating over an extended temperature range (up to +105 C), make the device particularly suitable for vibration monitoring in industrial applications. Datasheet: https://www.st.com/en/mems-and-sensors/iis3dwb.html This driver is currently only supporting the polling-mode read_and_decode APIs (both blocking and non-blocking). This driver is based on stmemsc HAL i/f v2.9.1. Signed-off-by: Armando Visconti <armando.visconti@st.com>

yperess

Haven't finished a full review, but this should be a good start

yperess · 2025-06-03T17:36:18Z

drivers/sensor/st/iis3dwb/iis3dwb.c

+	switch (fs) {
+	case 2:
+		range = IIS3DWB_DT_FS_2G;
+		break;
+	case 4:
+		range = IIS3DWB_DT_FS_4G;
+		break;
+	case 8:
+		range = IIS3DWB_DT_FS_8G;
+		break;
+	case 16:
+		range = IIS3DWB_DT_FS_16G;
+		break;
+	default:
+		LOG_ERR("fs [%d] not supported.", fs);
+		return -EINVAL;
+	}


While this works for all the sensors I know of we shouldn't be requiring exact values just like we don't require exact values for sample rates, if the used passed 1g for the scale, we should round up to 2 etc. Otherwise you get some strange behavior no? For example:

Passing 9.806650 m/s2 will result in 1g which will be invalid, but should be rounded to 2g.

yperess · 2025-06-03T17:37:29Z

drivers/sensor/st/iis3dwb/iis3dwb.c

+	default:
+		odr = IIS3DWB_XL_ODR_26k7Hz;


Other drivers only allow rounding up. We should keep that up, if the user asked for 25kHz then serving a higher value is OK, but if the user asks for 100kHz we should fail.

Here, the idea is that sensor can be either turned on at fixed rate (26.7 khz) or turned off. So, my idea was that the passed odr can be 0 for turning off or any value for turning on. I understand that there is a user API rule to be respected, so I may add a check.

yperess · 2025-06-03T17:39:38Z

drivers/sensor/st/iis3dwb/iis3dwb.c

+		read_reg = rtio_sqe_acquire(rtio);
+
+		if (write_addr == NULL || read_reg == NULL) {
+			return;


I think you need to mark iodev_sqe as failed here before returning

yperess · 2025-06-03T17:40:29Z

drivers/sensor/st/iis3dwb/iis3dwb.c

+
+	complete_op = rtio_sqe_acquire(rtio);
+	if (complete_op == NULL) {
+		return;


Same here, if you failed to start the next operation set you need to mark the incoming iodev_sqe as failed and release all the SQEs you've acquired

yperess · 2025-06-03T17:40:57Z

drivers/sensor/st/iis3dwb/iis3dwb.c

+	rc = rtio_sqe_rx_buf(iodev_sqe, min_buf_len, min_buf_len, &buf, &buf_len);
+	if (rc != 0) {
+		LOG_ERR("Failed to get a read buffer of size %u bytes", min_buf_len);
+		return;


Mark iodev_sqe as failed

yperess · 2025-06-03T17:42:54Z

drivers/sensor/st/iis3dwb/iis3dwb.c

+		}
+	}
+
+	if (edata->has_accel == 0) {


Won't this fail on SENSOR_CHAN_DIE_TEMP only reads? Do we have to read acceleration to get the die temperature?

No, you are right, we don't. We can read temp w/o reading accel.

On a second thought, I think it is not possible to have only temperature data. Nevertheless, I find useful to follow your suggestion anyway. Thx

yperess · 2025-06-03T17:43:50Z

drivers/sensor/st/iis3dwb/iis3dwb.c

+
+static DEVICE_API(sensor, iis3dwb_driver_api) = {
+	.attr_set = iis3dwb_attr_set,
+#ifdef CONFIG_SENSOR_ASYNC_API


I think you should use CONFIG_IIS3DWB_STREAM here

I think I require it even if STREAM is not defined, I see that 70% of the other drivers are exacting like this, BUT the remaining 30% does not wrap .get_decoder and .submit at all. I may follow that 30% of drivers

yperess · 2025-06-03T17:47:57Z

drivers/sensor/st/iis3dwb/iis3dwb.c

+	do {
+		iis3dwb_reset_get(ctx, &rst);
+	} while (rst);


This seems pretty aggressive, can we use WAIT_FOR here so there's a deadline?

ok, I'm looking at its usage

yperess · 2025-06-03T17:49:48Z

drivers/sensor/st/iis3dwb/iis3dwb.h

+
+#define GAIN_UNIT (61LL)
+
+#ifdef CONFIG_IIS3DWB_STREAM


Why hide this struct? these #if in headers make it really difficult to read the header and later make IS_ENABLED(CONFIG_IIS3DWB_STREAM) impossible to use

yperess · 2025-06-03T17:52:34Z

drivers/sensor/st/iis3dwb/iis3dwb.c

+			edata->has_accel = 1;
+
+			uint8_t outx_regs[]  = { IIS3DWB_OUTX_L_A, };
+			struct spi_buf buf_xl[] = { {(uint8_t *)edata->accel, 6} };


We shouldn't be using the memory from the driver here, the user should be providing the data with the SQE. If we have the MPU on, this would cause a fault.

But edata is not driver memory; it is allocated few lines above (buf). Can you comment on this?

avisconti · 2025-06-05T09:15:34Z

@yperess Thanks for your review. I'll take care of it as soon as possible. I think this work is important to consolidate and divulgate the read_and_decode approach. At this review stage I also want to ask you a comment about the iis3dwb_rtio_rd_transaction() routine. If you see among his argument there is a spi_buf structure pointer which basically it is used to pass a list of bufp and buf_len. It should be possible to prepare a RTIO buffer chunks reading, e.g. (using same concept in another driver I'm currently developing)

		uint8_t outx_regs[]  = { LSM6DSVXXX_WHO_AM_I, LSM6DSVXXX_OUTX_L_A };
		struct spi_buf buf_xl[] = { {&edata->chip_id, 1} , {(uint8_t *)edata->accel, 6}, };

		/*
		 * Prepare rtio enabled bus to read LSM6DSVXXX_OUTX_L_A register
		 * where accelerometer data is available.
		 * Then lsm6dsvxxx_one_shot_complete_cb callback will be invoked.
		 *
		 * STMEMSC API equivalent code:
		 *
		 *   uint8_t accel_raw[6];
		 *
		 *   lsm6dsvxxx_acceleration_raw_get(&dev_ctx, accel_raw);
		 */
		lsm6dsvxxx_rtio_rd_transaction(dev,
						outx_regs,
						ARRAY_SIZE(outx_regs),
						buf_xl,
						iodev_sqe,
						lsm6dsvxxx_one_shot_complete_cb);

Now I would like to know if

Would such routine be useful for everybody and hence include it as a new API in rtio.h?
Instead of using spi_buf (iis3dwb supports only SPI) we may use a different/custom styructure. We can define in rtio.h a new one.

If this can be a good idea I may implement it in this PR at this stage. Let me know.

EDIT:
Question open to everybody. Maybe @teburd may also have a comment

Add read_and_decode streaming APIs support. Triggers supported: - SENSOR_TRIG_FIFO_WATERMARK - SENSOR_TRIG_FIFO_FULL - SENSOR_TRIG_DATA_READY Signed-off-by: Armando Visconti <armando.visconti@st.com>

sonarqubecloud · 2025-06-12T14:20:03Z

Quality Gate passed

Issues
18 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
4.7% Duplication on New Code

See analysis details on SonarQube Cloud

teburd · 2025-06-12T14:30:42Z

@yperess Thanks for your review. I'll take care of it as soon as possible. I think this work is important to consolidate and divulgate the read_and_decode approach. At this review stage I also want to ask you a comment about the iis3dwb_rtio_rd_transaction() routine. If you see among his argument there is a spi_buf structure pointer which basically it is used to pass a list of bufp and buf_len. It should be possible to prepare a RTIO buffer chunks reading, e.g. (using same concept in another driver I'm currently developing)
		uint8_t outx_regs[]  = { LSM6DSVXXX_WHO_AM_I, LSM6DSVXXX_OUTX_L_A };
		struct spi_buf buf_xl[] = { {&edata->chip_id, 1} , {(uint8_t *)edata->accel, 6}, };

		/*
		 * Prepare rtio enabled bus to read LSM6DSVXXX_OUTX_L_A register
		 * where accelerometer data is available.
		 * Then lsm6dsvxxx_one_shot_complete_cb callback will be invoked.
		 *
		 * STMEMSC API equivalent code:
		 *
		 *   uint8_t accel_raw[6];
		 *
		 *   lsm6dsvxxx_acceleration_raw_get(&dev_ctx, accel_raw);
		 */
		lsm6dsvxxx_rtio_rd_transaction(dev,
						outx_regs,
						ARRAY_SIZE(outx_regs),
						buf_xl,
						iodev_sqe,
						lsm6dsvxxx_one_shot_complete_cb);
Now I would like to know if
1. Would such routine be useful for everybody and hence include it as a new API in rtio.h?

2. Instead of using spi_buf (iis3dwb supports only SPI) we may use a different/custom styructure. We can define in rtio.h a new one.
If this can be a good idea I may implement it in this PR at this stage. Let me know.

EDIT: Question open to everybody. Maybe @teburd may also have a comment

I think if I'm reading this correctly this API is wanting to do a series of register reads, e.g. read reg X, Y, and Z into values X, Y, Z?

I mean this is completely reasonable, and actually I think matches some ideas @ubieda had around a regmap like API on top of RTIO.

I'd welcome rtio_regmap.h or some such that provides an API like this to read/write to a register map over a bus easily and with the ability to control blocking/non-blocking like all other RTIO stuff.

avisconti · 2025-06-12T15:07:22Z

I think if I'm reading this correctly this API is wanting to do a series of register reads, e.g. read reg X, Y, and Z into values X, Y, Z?

I mean this is completely reasonable, and actually I think matches some ideas @ubieda had around a regmap like API on top of RTIO.

I'd welcome rtio_regmap.h or some such that provides an API like this to read/write to a register map over a bus easily and with the ability to control blocking/non-blocking like all other RTIO stuff.

Not sure we are talking about same thing. I will try to prepare a PR in the next week.

teburd

What a well written driver

Looking again the the bits here to read the interrupt status register and such though, I think this is very close to what @ubieda was thinking about. I'll try and suggest we discuss this in the next sensor wg if that makes sense to you @avisconti

avisconti · 2025-06-13T07:26:37Z

What a well written driver

Looking again the the bits here to read the interrupt status register and such though, I think this is very close to what @ubieda was thinking about. I'll try and suggest we discuss this in the next sensor wg if that makes sense to you @avisconti

Yes sure.

avisconti · 2025-06-13T07:34:37Z

@teburd @ubieda
I attach here the routine (taken from a new driver I'm implementing right now just to clarify what I'm talking about:

/*
 * Create a chain of SQEs representing a bus transaction to read a reg.
 * The RTIO-enabled bus driver will:
 *
 *     - write "reg" address
 *     - read "len" data bytes into "buf".
 *     - call complete_op callback
 */
void lsm6dsvxxx_rtio_rd_transaction(const struct device *dev,
				    uint8_t *regs, uint8_t regs_num,
				    struct spi_buf *buf,
				    struct rtio_iodev_sqe *iodev_sqe,
				    rtio_callback_t complete_op_cb)
{
	struct lsm6dsvxxx_data *lsm6dsvxxx = dev->data;
	struct rtio *rtio = lsm6dsvxxx->rtio_ctx;
	struct rtio_iodev *iodev = lsm6dsvxxx->iodev;
	struct rtio_sqe *write_addr;
	struct rtio_sqe *read_reg;
	struct rtio_sqe *complete_op;
	uint8_t i;

	for (i = 0; i < regs_num; i++) {

		write_addr = rtio_sqe_acquire(rtio);
		read_reg = rtio_sqe_acquire(rtio);

		if (write_addr == NULL || read_reg == NULL) {
			return;
		}

		if (lsm6dsvxxx->bus_type == BUS_SPI) {
			regs[i] |= 0x80; /* mark the SPI read transaction */
		}

		rtio_sqe_prep_tiny_write(write_addr, iodev, RTIO_PRIO_NORM, &regs[i], 1, NULL);
		write_addr->flags = RTIO_SQE_TRANSACTION;
		rtio_sqe_prep_read(read_reg, iodev, RTIO_PRIO_NORM, buf[i].buf, buf[i].len, NULL);
		read_reg->flags = RTIO_SQE_CHAINED;

		if (lsm6dsvxxx->bus_type == BUS_I2C) {
			read_reg->iodev_flags |= RTIO_IODEV_I2C_STOP | RTIO_IODEV_I2C_RESTART;
		} else if (lsm6dsvxxx->bus_type == BUS_I3C) {
			read_reg->iodev_flags |= RTIO_IODEV_I3C_STOP | RTIO_IODEV_I3C_RESTART;
		}
	}

	complete_op = rtio_sqe_acquire(rtio);
	if (complete_op == NULL) {
		return;
	}

	rtio_sqe_prep_callback_no_cqe(complete_op, complete_op_cb, (void *)dev, iodev_sqe);

	rtio_submit(rtio, 0);
}

I noticed that a routine like that can be generally useful for every driver. It will be called in this way:

			uint8_t outx_regs[]  = { data->out_xl, };
			struct spi_buf buf_xl[] = { {(uint8_t *)edata->accel, 6} };

			/*
			 * Prepare rtio enabled bus to read LSM6DSVXXX_OUTX_L_A register
			 * where accelerometer data is available.
			 * Then lsm6dsvxxx_one_shot_complete_cb callback will be invoked.
			 *
			 * STMEMSC API equivalent code:
			 *
			 *   uint8_t accel_raw[6];
			 *
			 *   lsm6dsvxxx_acceleration_raw_get(&dev_ctx, accel_raw);
			 */
			lsm6dsvxxx_rtio_rd_transaction(dev,
							outx_regs,
							ARRAY_SIZE(outx_regs),
							buf_xl,
							iodev_sqe,
							lsm6dsvxxx_one_shot_complete_cb);

Please note that spi_buf is not what I woul want to do (bus can be I2C or I3C as well).
We should define a generic but similar structure with bufp and len chunks

avisconti · 2025-06-17T13:39:04Z

This PR might be changed according to what decision will be taken on #91775,

avisconti added area: Drivers area: Sensors Sensors area: Devicetree Binding PR modifies or adds a Device Tree binding area: RTIO labels May 21, 2025

github-actions bot requested review from asemjonovs, jeppenodgaard, MaureenHelm, teburd, tristan-google, ubieda and yperess May 21, 2025 13:49

github-actions bot assigned MaureenHelm May 21, 2025

avisconti force-pushed the add-iis3dwb branch 2 times, most recently from 76587b9 to 6540c50 Compare May 22, 2025 15:24

teburd reviewed May 22, 2025

View reviewed changes

avisconti force-pushed the add-iis3dwb branch from 6540c50 to f2743b9 Compare May 26, 2025 14:50

avisconti requested a review from teburd May 26, 2025 15:53

avisconti force-pushed the add-iis3dwb branch from f2743b9 to d8db456 Compare June 3, 2025 15:41

yperess requested changes Jun 3, 2025

View reviewed changes

drivers/sensor: iis3dwb: add streaming capabality

f0010b2

Add read_and_decode streaming APIs support. Triggers supported: - SENSOR_TRIG_FIFO_WATERMARK - SENSOR_TRIG_FIFO_FULL - SENSOR_TRIG_DATA_READY Signed-off-by: Armando Visconti <armando.visconti@st.com>

avisconti force-pushed the add-iis3dwb branch from d8db456 to f0010b2 Compare June 12, 2025 13:55

avisconti requested a review from yperess June 12, 2025 13:55

teburd approved these changes Jun 13, 2025

View reviewed changes

Add read_and_decode API based iis3dwb sensor driver #90286

Are you sure you want to change the base?

Add read_and_decode API based iis3dwb sensor driver #90286

Conversation

avisconti commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yperess left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avisconti Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avisconti commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Jun 12, 2025

Quality Gate passed

Uh oh!

teburd commented Jun 12, 2025

Uh oh!

avisconti commented Jun 12, 2025

Uh oh!

teburd left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avisconti commented Jun 13, 2025

Uh oh!

avisconti commented Jun 13, 2025

Uh oh!

avisconti commented Jun 17, 2025

Uh oh!

Uh oh!

avisconti commented May 21, 2025 •

edited

Loading

avisconti Jun 12, 2025 •

edited

Loading

avisconti commented Jun 5, 2025 •

edited

Loading

teburd left a comment •

edited

Loading